Skip to content

Fix coverage commenting#77

Merged
kaitj merged 2 commits into
mainfrom
maint/ci-coverage
May 20, 2026
Merged

Fix coverage commenting#77
kaitj merged 2 commits into
mainfrom
maint/ci-coverage

Conversation

@kaitj
Copy link
Copy Markdown
Contributor

@kaitj kaitj commented May 11, 2026

Splits the commenting from the CI by:

  1. Performing the testing and uploading the coverage files in ci.yaml workflow
  2. Downloading coverage and commenting, never checking out the code (coverage.yaml) workflow.

This should allow for non-organization contributors to make PRs while still facilitating the coverage commenting without requiring the change of trigger from "pull_request" to "pull_request_target", which may introduce security vulnerabilities targeting the tests.

@nx10, mind taking a quick look at this one. The only part I am not certain about is the downloading of the artifact and if it ends up grabbing the correct PR number (i.e. artifact name) in the the subsequent workflow. Primarily needed some way of ensuring each artifact was unique to the PR.

Splits the commenting from the CI by:
  1. Performing the testing and uploading the coverage files in ci.yaml workflow
  2. Downloading coverage and commenting, never checking out the code.

This should allow for contributors (not part of the organization) to make PRs while still facilitating the coverage commenting without requiring the change of trigger from "pull_request" to "pull_request_target", which may introduce security vulunerabilities targetting the tests.
@kaitj kaitj requested a review from nx10 May 11, 2026 14:12
- Pass run-id/github-token so download-artifact reaches the CI run
- Pipe PR number through pr-number.txt (workflow_run.pull_requests is
  empty for fork PRs)
- Pass issue-number explicitly to the comment action
- Gate coverage job on pull_request event + successful CI conclusion
- Restore COVERAGE_PYTHON env var; cap artifact retention at 1 day
@nx10
Copy link
Copy Markdown
Collaborator

nx10 commented May 20, 2026

tweaked a few things - did you try this on a fork?

@kaitj
Copy link
Copy Markdown
Contributor Author

kaitj commented May 20, 2026

tweaked a few things - did you try this on a fork?

Haven't tried it yet - I don't think it would get triggered here anyways until this is merged in anyways because of:

on:
  push:
    branches: ["main"]
  pull_request:
    branches: ["main"]

@kaitj kaitj merged commit 8cee006 into main May 20, 2026
5 checks passed
@kaitj kaitj deleted the maint/ci-coverage branch May 20, 2026 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants